-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: If statement initializers #23
base: master
Are you sure you want to change the base?
RFC: If statement initializers #23
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
if local b = foo() where b == false then | ||
print(b) | ||
else | ||
print("true") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually gave me a motivating reason why it might be useful for b
to be visible in the else
branch. Say you have some animal:
type Dog = { tag: "dog", pet: true, name: string }
type Cat = { tag: "cat", pet: true, name: string }
type Sheep = { tag: "sheep", pet: false }
type Animal = Dog | Cat | Sheep
if local animal = getAnimal() where animal.pet then
print(`Hello {animal.name}!`)
else
print(`{animal.tag} is not a pet`) -- generalizes to all current and future `animal.tag`s
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another case:
if local success, msg = pcall(f) where success then
print(`all went well`)
else
print(`f failed with: {msg}`)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually gave me a motivating reason why it might be useful for
b
to be visible in theelse
branch. Say you have some animal:type Dog = { tag: "dog", pet: true, name: string } type Cat = { tag: "cat", pet: true, name: string } type Sheep = { tag: "sheep", pet: false } type Animal = Dog | Cat | Sheep if local animal = getAnimal() where animal.pet then print(`Hello {animal.name}!`) else print(`{animal.tag} is not a pet`) -- generalizes to all current and future `animal.tag`s end
Another case:
if local success, msg = pcall(f) where success then print(`all went well`) else print(`f failed with: {msg}`) end
Seeing it now allowing b
to be visible in the else
branch (and most likely elseif
branches) would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO pcall
is the most compelling reason to scope the locals this way, but this scoping behavior is a consistent footgun in C++ where bindings in conditions are scoped to the whole if statement and you can trivially end up dereferencing null pointers accidentally using them in the wrong branch. I think, outside of pcall
, scoping the bindings to the whole block means making more bugs with accidentally using nil
possible, and I see little benefit since you can already write the following to have this scoping behavior.
do local success, msg = pcall(f)
if success then
print(`all went well`)
else
print(`f failed with: {msg}`)
end
end
|
||
`Output: true` | ||
|
||
When declaring multiple values inside of a condition, all of the variables will be evaluated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for clarification, how would the variables be combined when being tested? I misread the example at first, so I assumed all variables are combined with or
, but it actually implies using and
.
if local a, b = true, false then
print("pass")
else
print("fail")
end
-- same as this?
if local a, b = true, false where a and b then
print("pass")
else
print("fail")
end
I want to argue that this evaluation is ambiguous, since there is no explicit operation between the variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're both the same, it isn't ambiguous at all. Requiring that only one value is truthy removes a lot of the uses it brings and requires you to include checks to make sure your variables aren't false (or true).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've come to agree with @vegorov-rbx on making where
clause required if more than two locals are involved in the initializer. Good example is found here: https://github.com/luau-lang/rfcs/pull/23/files#r1492770178.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think where clauses should just always be a requirement 😰
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented this elsewhere, but I realized this is pretty much a good point to say it in. The desirable behavior IMO is that any variable initialized in an if local
statement will be tested to be non-nil
. The direct argument would be that putting the initialization in a condition is an indication that you want it to be bound to some sort of non-nil
value in the block following it. The forward-looking advantage of this is that it's consistent with how destructuring should behave, e.g. if we allow destructuring tables in local definitions (#24), you could write something like:
if local {.x, .y, .z} = point then
-- x, y, z are defined and non-`nil`
end
The main downside is that it means the case where you specifically want to test a result for truthiness, rather than non-nil
ness will require you to write the condition clause, i.e.
if local cond = bool() then
-- cond is not `nil`, but could be `true` or `false`
end
if local cond = bool() in cond then
-- cond is not `nil`, and has been tested `true` by the condition clause
end
if local cond = bool() in not cond then
-- cond is not `nil`, and has been tested `false` by the condition clause
end
I think this is a reasonable decision to make. Requiring in
is a more conservative choice that is forwards-compatible with this evolution though.
I would be curious about the interaction with this RFC and key destructuring (#24). Right now, I believe we should disallow key destructuring in If we allowed it, then we'd have to decide on the right semantics, which might motivate us to disallow key destructuring in if local {.x, .y} = f() then
print(x, y)
end Which of the following best matches your intuition on what happens?
This is kind of fine, but what happens if
This would fix the problem where the condition blows up, which I'd think is an improvement. But now, this means the VM would need to test the type of If we ever allowed key destructuring, I think the second option makes the most sense because of how much code you avoid having to write in order to simulate the second semantics: local t_ok = false
do
local t = f()
-- the condition to type test whether `t` can be key destructured
if type(t) == "table" or type(t) == "userdata" or type(t) == "vector" or type(t) == "string" then
local {.x, .y} = t
-- the condition of the `where` clause
if x and y then
-- if both type test and where clause succeeds, then `t_ok` is true
t_ok = true
-- ...
end
end
end
-- if either type test or `where` clause failed, this is the `else` branch
if not t_ok then
-- ...
end So yeah, it's very easy to start by just not allowing key destructuring if that RFC is ratified. |
Hot take: make |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This could work, I didn't consider key destructuring (nor am I particularly a fan of it in the first place). Making |
Co-authored-by: Alexander McCord <[email protected]>
Just ran into something relatively verbose where this RFC (almost) shines. function PlayerControl:GetEntities()
if self.RemovedSelf then
return self.Instances
end
local entity = self:TryGetEntity()
if entity == nil then
return self.Instances
end
local index = table.find(self.Instances, entity.Root)
if index == nil then
return self.Instances
end
table.remove(self.Instances, index)
self.RemovedSelf = true
return self.Instances
end Could be compacted into function PlayerControl:GetEntities()
if self.RemovedSelf then
elseif local entity = self:TryGetEntity() where entity == nil then
elseif local index = table.find(self.Instances, entity.Root) then
table.remove(self.Instances, index)
self.RemovedSelf = true
end
return self.Instances
end This would make a great addition to this RFC in the motivation section. |
This comment was marked as resolved.
This comment was marked as resolved.
Could an alternative solution be Rewriting the above example: function PlayerControl:GetEntities()
if not self.RemovedSelf then
local entity = self:TryGetEntity()
andif entity ~= nil then
-- still in the same scope, or has access to previous scope
local index = table.find(self.Instances, entity.Root)
andif index then
table.remove(self.Instances, index)
self.RemovedSelf = true
else
-- false for any andif will go here
-- convenient warning/error place, could return here too
end
return self.Instances
end``` |
This comment was marked as resolved.
This comment was marked as resolved.
The example you provided isn't an initializer but a completely separate alternative from the proposed syntax. |
Yes I was commenting an alternative to the proposed syntax, that was not a mistake. My reasoning was based on the motivating examples. I think what we really wanted to do to those is abandon certain behavior if a chain of necessities wasn't met; every alternative syntax I could think of wouldn't work with the examples because eventually what was necessary was a testing condition and a naked statement to execute depending on the condition... ideally a common naked statement that you only type once. by having a chain of local function one_dependency()
if local a, b = foo() where a and b then
end
end
local function one_dependency_andif()
local a,b = foo()
if a and b then
end
end
local function two_dependencies()
if local a, b = foo() where a and b then
elseif local c, d = a(), b() where c and d then
end
end
local function two_dependencies_andif()
local a,b = foo()
if a and b then
local c, d = a(), b()
andif c and d then
end
end Now that I think of it, I guess the two ideas don't really compete: local function two_dependencies_both()
if local a,b = foo() where a and b then
andif local c, d = a(), b() where c and d then
end
end``` |
Co-authored-by: Alexander McCord <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
We've extensively discussed this RFC on the OSS Discord lately, and I want to update this page on some major points from those discussions. To summarize the main changes to the RFC proposal:
motivationsAccording to aaron, multiple other luau language engineers, and the majority of active chat participants I see discussing this over there:
At the moment, this RFC can be described as "an ergonomic wrapper around This is not what the Luau engineers seem to want to implement. To quote aaron:
In general, the engineers seem extremely interested in implementing fallthroughThe general consensus seems to be "you can already do this with a
To quote WheretIB:
If the language engineers themselves would rather just not have the feature than implement it with fallthrough, then it makes the most sense to me to listen to them and figure out what they'd intend it for and why. footgunAdditionally, I argue that the currently-proposed fallthrough behavior is actually a footgun: it encourages minimized LOC, never-nesting, and excessive DRY at the expense of code readability. I was a pretty vocal proponent for fallthrough until around 2 weeks ago. This is why I've changed my mind. I've spent too many hours these last few months writing and grok-testing real life usecases for Nesting conditionals is not necessarily a bad thing. It allows programmers to easily parse what the control flow is doing at any given point, what variables are in scope and where they're defined and utilized, just by lining up indentation. On the other hand, allowing Here's a rewrite of my hit validation example which shows how function Caster:VerifyTargetHit(hit: Instance): (Humanoid?, Player?)
if local hitModel = hit.Parent in hitModel:IsA("Model") then
if local humanoid = hitModel:FindFirstChildOfClass("Humanoid") in
humanoid and humanoid.Health > 0
then
if hitModel:HasTag("EnemyBot") then
return humanoid, nil
elseif local player = Players:GetPlayerFromCharacter(hitModel) in
player and player.Team ~= LocalPlayer.Team
then
return humanoid, player
end
end
end
end new functionalityThe RFC, as currently proposed, doesn't properly introduce this new functionality to the language: if local result = someExpensiveCall() in result and result.ok then
-- bind result *only* within this branch
end There is currently no way to express this in Luau:
By limiting the scope of the bound identifier to a single branch, we can introduce this brand new behavior and simultaneously improve the ergonomics, readability, and scopeability (is that even a word) of one of the most common Lua/u idioms ever: -- Assign a result to an identifier. do something if it's truthy.
local container = SomeClass:AnnoyinglyLongMethod("meow")
if container then
-- do things with container
end
-- ignore the now bound container forever
-- or
if SomeClass:AnnoyinglyLongMethod("meow") then
local container = SomeClass:AnnoyinglyLongMethod("meow")
-- yay we're now bound only in here!
-- hope that wasn't an expensive method call (or caused side effects) cause we just called it twice
-- do things with container
end Which can now be written: if local container = SomeClass:AnnoyinglyLongMethod("meow") then
-- do things with container knowing it happily exists
end
-- what container?? never heard of her I might be extremely biased here, but just this basic
|
`where` ~> `in`
When declaring multiple values inside of an initializer, the `in` clause is required. | ||
|
||
Example: | ||
|
||
```lua | ||
local function foo() | ||
return true, false | ||
end | ||
|
||
if local a, b = foo() in a and b then | ||
else | ||
print'Hello World, from Luau!' | ||
end | ||
``` | ||
|
||
`Output: Hello World, from Luau!` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With an eye towards consistent behavior with destructuring in the future, I would prefer to see this changed to requiring a
and b
be non-nil implicitly, but it's ultimately okay if we air on the conservative side here since this requirement is forwards-compatible with changing it in the future to mean that. In general, I think the behavior we want though is that using local a = foo()
and local a, b = foo()
in a condition will only ever take that branch when the results are non-nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. If you want to allow b
to either exist or not, you should probably have to say that explicitly. Otherwise the fact that you asked for local a, b = foo()
probably means that you wanted both a
and b
to both exist.
if local b = foo() then | ||
print(b, "truthy block") | ||
else | ||
print(b, "falsy block") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanted to add that this design point is one we have to live with in perpetuity. If we decided to take this fallthrough semantics out of this RFC, then we're stuck with that behavior.
Using boolean
is an ok example, but it doesn't strongly argue for it as opposed to some other example I posted earlier wrt animal
and one that @vegorov-rbx posted with pcall
example. If a design point is a "choose now or forever hold your peace," it especially needs an equally as compelling argument to support it.
So I'd rather block the RFC until we are able to agree on this design point, whether locals should fallthrough or not.
An example where the behavior is breaking change would also be good to include in the alternatives section.
if local x = f() in x % 2 == 0 then
print(x) --> any even numbers
else
print(x) --> nil
end
As is, this RFC proposes the print(x)
in the else
branch to print any odd numbers.
I've very quickly skimmed the comments but could we drop the need for if x = f() and x == 5 then
end Is just sugar for: local x = f()
if x == 5 then
end This works nicely with the fall-through if success, result = pcall(...) then
-- success here
else
-- failure here
warn(result)
end Is sugar for: local success, result = pcall(...)
if success then
-- success here
else
-- failure here
warn(result)
end |
This defeats the point of the RFC, and, as we've stated earlier, causes confusion between assignments (=) and equality checks (==). The new syntax needs to be obvious and explicit; The purpose of this RFC is to provide syntax to
The main usecase is any extremely common idiom like the following: -- with the implicit "in binding ~= nil" clause:
if local humanoid = hit:FindFirstChildOfClass("Humanoid") then
-- binds local humanoid and executes branch if humanoid ~= nil
end
-- local humanoid is unbound here
-- or with an explicit "in" clause:
if local result = getSomeResult() in table.find(validOptions, result) then
end At the moment, we kinda just need to agree upon fallthrough semantics to get this RFC passed. |
I am not convinced that requiring
I understand the benefits this feature would bring but I think we need to be careful about the other "gotchas" we might introduce as part of it. I would like to see some more alternatives before we move forward with this. |
edits: removed my point 2 "counterargument": not objectively reasonable; added sentence.
This feature combines the existing and well-known |
If we were only to ever do this for Unfortunately, I also don't have a great solution to this. If we had a if var x = foo() and x == y then
while var x = foo() and x do
for var x = 1, 5 do At least in this case the semantics seem a little more reasonable to me. |
I can imagine us doing I would even say I'm surprised about some "language edition" talks that include replacing that keyword, feels like people don't want to work on Luau and want to work on a different language and use editions as an excuse to pretend that it would be the same language with 'fixes'. |
Agreed, I do not think that the fact that |
Definitely agreed; imho |
Sorry I've been busy. I'm fine with if local a,b = foo() in (a == "hello") and (b == "world") then
print(a,b) --> "hello", "world"
else
print(a,b) --> nil, nil
end
print(a,b) --> nil, nil vs if local a,b = foo() in (a == "hello") and (b == "world") then
print(a,b) --> "hello", "world"
else
print(a,b) --> "something", "else"
end
print(a,b) --> nil, nil Since the RFC hasn't moved at all I am fine with the former syntax being the case rather than the latter, which is the current way. |
Rendered